Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Blocks: Calendly block #14252

Merged
merged 3 commits into from
Jan 16, 2020
Merged

Blocks: Calendly block #14252

merged 3 commits into from
Jan 16, 2020

Conversation

scruffian
Copy link
Member

@scruffian scruffian commented Dec 17, 2019

This adds a new Jetpack block for Calendly.

Changes proposed in this Pull Request:

  • The block supports all the different embed options that Calendly offer, as well as pasting the embed code.

Screenshots:
Screenshot 2020-01-13 at 18 04 53
Screenshot 2020-01-13 at 18 05 08
Screenshot 2020-01-13 at 18 05 34

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Master Thread: pb5gDS-fs-p2

Testing instructions:

  • Enable Beta Blocks
  • Create a new post
  • Add a Calendly block
  • Paste in each of the following:
  • A Calendly URL
  • A Calendly URL without https://
  • The inline embed code
  • The popup widget embed code
  • The text widget embed code
  • Repeat the previous steps but customise the colours (requires a pro account)
  • Edit the block from the sidebar:
  • Styles
  • Hide event details
  • Update the block using the embed code
  • Check that the block renders correctly on the user's site

Using Calendly

To get the different embed codes from Calendly you will need a Calendly account:

Screenshot 2020-01-13 at 17 54 59

Screenshot 2020-01-13 at 17 54 54

Proposed changelog entry for your changes:

  • Add Calendly block

@jetpackbot
Copy link

jetpackbot commented Dec 17, 2019

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: February 11, 2020.
Scheduled code freeze: February 4, 2020

Generated by 🚫 dangerJS against e9969e2

@jeherve jeherve added [Block] Calendly [Type] Feature Request [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Dec 17, 2019
@jeherve jeherve changed the title Add/calendly block Blocks: new Calendly block Dec 17, 2019
@huguesvincent huguesvincent added this to the 8.2 milestone Dec 18, 2019
@glendaviesnz
Copy link
Contributor

glendaviesnz commented Jan 9, 2020

One minor thing - if a user just highlights their link and cmd-v's it instead of clicking the 'copy' button, which they may not even realise is a copy button, then the embed fails as it is missing the https:// - would be better to allow the url with or without the https://

calendly

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Jan 9, 2020

Nice work 👍 Apart from the issue mentioned with the link without http it seemed to work well for me.

The only thing I couldn't work out is what difference the 'Hide event type details' was supposed to make - didn't seem to have any effect, but that may just be to do with how my calendly account is set up.

Would be good to add some unit tests for the likes of getUrlAndStyleFromEmbedCode & getNewAttributesFromUrl if the api for these is firmed up, and maybe these could be pulled out into a util module - but that is maybe a phase 2 job?

@huguesvincent
Copy link

then the embed fails as it is missing the https:// - be better to allow the url with or without the https://
thanks for spotting this @glendaviesnz

@scruffian
Copy link
Member Author

'Hide event type details' does this:
Screenshot 2020-01-13 at 17 36 09
Screenshot 2020-01-13 at 17 35 31

Maybe this is a pro feature?

@scruffian scruffian changed the title Blocks: new Calendly block Blocks: Calendly block Jan 13, 2020
@scruffian scruffian added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Jan 13, 2020
@glendaviesnz
Copy link
Contributor

'Hide event type details' does this: Maybe this is a pro feature?

Working for me now - I had embedded the list of all events rather than a single event, so the toggle had no effect, but works when embedding a specific event.

@scruffian scruffian force-pushed the add/calendly-block branch 2 times, most recently from 4318050 to 473c59e Compare January 13, 2020 20:42
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way we can make the link preview look a bit better?
image


Maybe we can do something to error out nicely when one does not paste a proper embed code (like for example another type of Calendly embed)?

image

extensions/blocks/calendly/attributes.js Outdated Show resolved Hide resolved
extensions/blocks/calendly/calendly.php Outdated Show resolved Hide resolved
extensions/blocks/calendly/calendly.php Outdated Show resolved Hide resolved
extensions/blocks/calendly/calendly.php Outdated Show resolved Hide resolved
extensions/blocks/calendly/calendly.php Outdated Show resolved Hide resolved
extensions/blocks/calendly/index.js Outdated Show resolved Hide resolved
extensions/blocks/calendly/index.js Outdated Show resolved Hide resolved
extensions/blocks/calendly/index.js Show resolved Hide resolved
extensions/blocks/calendly/index.js Outdated Show resolved Hide resolved
extensions/blocks/calendly/test/utils.js Show resolved Hide resolved
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 14, 2020
@scruffian
Copy link
Member Author

scruffian commented Jan 15, 2020

I have addressed the major concerns on the PR. I would like to do some refactoring to reuse some code from the OpenTable PR, however, is it possible to merge this PR as is and follow with that in a future PR? IMO This would make the review process simpler.

extensions/blocks/calendly/edit.js Outdated Show resolved Hide resolved
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good now. I'll merge it for now. I've opened issues for all issues that I think are left right now:

#14365
#14366
#14367
#14368

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Jan 16, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello scruffian! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D37786-code before merging this PR. Thank you!

@jeherve jeherve merged commit 2c3dbd9 into master Jan 16, 2020
@jeherve jeherve deleted the add/calendly-block branch January 16, 2020 14:37
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jan 16, 2020
@jeherve jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Jan 16, 2020
jeherve added a commit that referenced this pull request Jan 17, 2020
jeherve added a commit that referenced this pull request Jan 28, 2020
* [not verified] Remove empty readme section

* Initial changelog for 8.2

* Changelog: add #14220

* Changelog: add #14252

* Changelog: add #14291

* Changelog: add #14309

* Changelog: add #14304

* Changelog: add general connection log.

* Changelog: add #14275

* Changelog: add #14313

* Changelog: add #14213

* Changelog: add #14357

* Add sync testing instructions

* Add 8.1.1 changelog back

See eeaafab and 61757eb

* Changelog: add #14371

* Changelog: add #14386

* Changelog: add #14471

* Changelog: add #14325

* Changelog: add #14194

* Changelog: add #14340

* Changelog: add #14418

* Changelog: add #14417

* Changelog: add #14075

* Changelog: add #14467

* Changelog: add #14307

* Changelog: add #14326
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Calendly [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Feature Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants